[Enhancement-3191] `transport_enabled` setting on an auth domain and authorizer may be unnecessary after transport client removal #3939

prabhask5 · 2024-01-11T06:34:10Z

Description

Remove the transport_enabled setting in config.yml and its related code in DynamicConfigModelV6/7, ConfigV6/7, associated tests. Additionally add handlers in Config files to handle when transport_enabled is still used within the config.yml file while alerting the user with a message in startup that this setting can be safely removed from the config.

Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
Enhancement
Why these changes are required?
transport_enabled setting on an auth domain and authorizer may be unnecessary after transport client removal #3191 states that since the transport client has been removed, there is no need for the transport_enabled config and its related code to exist in the codebase, so this ticket removes that unnecessary code.
What is the old behavior before changes and new behavior after changes?
Nothing.

Issues Resolved

#3191

Is this a backport? If so, please add backport PR # and/or commits #
No

Testing

No tests were made, all tests should run since the behavior should not change.

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Prabhas Kurapati <[email protected]>

codecov · 2024-01-12T13:37:40Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (f217fa8) 65.18% compared to head (ba197b7) 65.61%.
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3939      +/-   ##
==========================================
+ Coverage   65.18%   65.61%   +0.43%     
==========================================
  Files         298      298              
  Lines       21216    21246      +30     
  Branches     3457     3457              
==========================================
+ Hits        13829    13940     +111     
+ Misses       5672     5588      -84     
- Partials     1715     1718       +3

Files	Coverage Δ
...ch/security/securityconf/DynamicConfigModelV7.java	`65.51% <100.00%> (-0.79%)`	⬇️
...pensearch/security/setting/DeprecatedSettings.java	`85.71% <100.00%> (+5.71%)`	⬆️
...ch/security/securityconf/DynamicConfigModelV6.java	`0.00% <0.00%> (ø)`
...search/security/securityconf/impl/v7/ConfigV7.java	`78.39% <83.33%> (+1.42%)`	⬆️
...search/security/securityconf/impl/v6/ConfigV6.java	`62.39% <66.66%> (+2.58%)`	⬆️

... and 9 files with indirect coverage changes

Signed-off-by: Prabhas Kurapati <[email protected]>

prabhask5 · 2024-01-12T14:10:23Z

@peternied @scrawfor99 @cwperks Ready for review!

peternied

Thanks, lets reuse DeprecatedSettings and add more test cases, see comments inline.

src/main/java/org/opensearch/security/securityconf/impl/v6/ConfigV6.java

stephen-crawford

Hi @prabhask5 thanks for making these changes. Great job keeping things up to date! Other than Peter's suggestions, things look good to me. Let me know when you address those and I can give a final pass through and approve (assuming nothing else comes up etc.)

DarshitChanpura

Changes look good to me. Have we analyzed the blast radius for removing these files? Will there be downstream plugins that will have broken tests?

prabhask5 · 2024-01-18T12:30:16Z

Changes look good to me. Have we analyzed the blast radius for removing these files? Will there be downstream plugins that will have broken tests?

@DarshitChanpura I haven't checked for any other repository tests being impacted by this change, though I can confirm that the only errors that were caused by removing this setting being a parsing error from Jackson which I resolved using the unknownPropertiesHandler. If this error would have occurred in downstream plugins it should not. I'm new to contributing, can you let me know of other plugins to check tests for?

Signed-off-by: Prabhas Kurapati <[email protected]>

prabhask5 · 2024-01-18T14:53:25Z

@peternied I think I've addressed all your requested changes:

I added a new method to the DeprecatedSettings class that allows for the logging of any deprecated setting in which the codebase does not support any more and the utilization of would result in an error, I have "extra information" of where the config that should be removed as a parameter to add extra information for a specific setting.
Previously the unknownPropertyHandler didn't account for other unknown properties so I refactored it so it throws the original error if the setting is not transport_enabled (which is currently the only setting that is deprecated here), I also used a switch statement to make it easy to add more deprecated settings in the future.
I added some more tests in DeprecatedSettingsTest to account for both of these cases.

This should be ready for review once I fix the test errors!

Signed-off-by: Prabhas Kurapati <[email protected]>

stephen-crawford

Hi @prabhask5, thanks for your work with this! Looks good to me :)

DarshitChanpura · 2024-01-18T15:20:54Z

@DarshitChanpura I haven't checked for any other repository tests being impacted by this change, though I can confirm that the only errors that were caused by removing this setting being a parsing error from Jackson which I resolved using the unknownPropertiesHandler. If this error would have occurred in downstream plugins it should not. I'm new to contributing, can you let me know of other plugins to check tests for?

I'm not aware of any off the top of my mind. We can merge this and if there are any downstream failures we can analyze and revert of necessary.

src/main/java/org/opensearch/security/setting/DeprecatedSettings.java

Signed-off-by: Prabhas Kurapati <[email protected]>

…authorizer may be unnecessary after transport client removal (#3939) Signed-off-by: Prabhas Kurapati <[email protected]> (cherry picked from commit 881ed58) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…uth domain and authorizer may be unnecessary after transport client removal (#3966) Backport 881ed58 from #3939. Signed-off-by: Prabhas Kurapati <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…authorizer may be unnecessary after transport client removal (opensearch-project#3939) Signed-off-by: Prabhas Kurapati <[email protected]>

prabhask5 added 7 commits January 11, 2024 12:01

remove code

1bf104b

Signed-off-by: Prabhas Kurapati <[email protected]>

remove more instances of transport_enabled

aa15cd5

Signed-off-by: Prabhas Kurapati <[email protected]>

remove more instances of transport_enabled

11aba2f

Signed-off-by: Prabhas Kurapati <[email protected]>

undo config changes

6be4f75

Signed-off-by: Prabhas Kurapati <[email protected]>

handle unknown transport_enabled setting in serialization

73a0f0f

Signed-off-by: Prabhas Kurapati <[email protected]>

removed print statements

364a21c

Signed-off-by: Prabhas Kurapati <[email protected]>

remove transport_enabled from json files to fix test errors

02abe78

Signed-off-by: Prabhas Kurapati <[email protected]>

change set up logging test

b63afa5

Signed-off-by: Prabhas Kurapati <[email protected]>

prabhask5 marked this pull request as ready for review January 12, 2024 14:09

prabhask5 requested review from cliu123, cwperks, DarshitChanpura, davidlago, peternied, RyanL1997, stephen-crawford, reta and willyborankin as code owners January 12, 2024 14:09

peternied requested changes Jan 12, 2024

View reviewed changes

stephen-crawford reviewed Jan 12, 2024

View reviewed changes

DarshitChanpura reviewed Jan 16, 2024

View reviewed changes

add requested changes

22f25f7

Signed-off-by: Prabhas Kurapati <[email protected]>

spotless apply

5609519

Signed-off-by: Prabhas Kurapati <[email protected]>

stephen-crawford previously approved these changes Jan 18, 2024

View reviewed changes

prabhask5 requested review from DarshitChanpura and peternied January 18, 2024 15:13

prabhask5 requested a review from stephen-crawford January 18, 2024 15:13

DarshitChanpura previously approved these changes Jan 18, 2024

View reviewed changes

cwperks reviewed Jan 18, 2024

View reviewed changes

src/main/java/org/opensearch/security/setting/DeprecatedSettings.java Outdated Show resolved Hide resolved

use version class to get current version of opensearch

876e294

Signed-off-by: Prabhas Kurapati <[email protected]>

prabhask5 dismissed stale reviews from DarshitChanpura and stephen-crawford via 876e294 January 18, 2024 18:45

spotless apply

ba197b7

Signed-off-by: Prabhas Kurapati <[email protected]>

peternied approved these changes Jan 18, 2024

View reviewed changes

prabhask5 requested review from cwperks and DarshitChanpura January 19, 2024 01:20

cwperks approved these changes Jan 19, 2024

View reviewed changes

cwperks added the backport 2.x backport to 2.x branch label Jan 19, 2024

peternied merged commit 881ed58 into opensearch-project:main Jan 19, 2024
83 checks passed

opensearch-trigger-bot bot mentioned this pull request Jan 19, 2024

[Backport 2.x] [Enhancement-3191] transport_enabled setting on an auth domain and authorizer may be unnecessary after transport client removal #3966

Merged

prabhask5 deleted the pk-remove-transport-enabled-config branch July 10, 2024 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enhancement-3191] `transport_enabled` setting on an auth domain and authorizer may be unnecessary after transport client removal #3939

[Enhancement-3191] `transport_enabled` setting on an auth domain and authorizer may be unnecessary after transport client removal #3939

prabhask5 commented Jan 11, 2024

codecov bot commented Jan 12, 2024 •

edited

Loading

prabhask5 commented Jan 12, 2024

peternied left a comment

stephen-crawford left a comment

DarshitChanpura left a comment

prabhask5 commented Jan 18, 2024 •

edited

Loading

prabhask5 commented Jan 18, 2024

stephen-crawford left a comment

DarshitChanpura commented Jan 18, 2024 •

edited

Loading

[Enhancement-3191] transport_enabled setting on an auth domain and authorizer may be unnecessary after transport client removal #3939

[Enhancement-3191] transport_enabled setting on an auth domain and authorizer may be unnecessary after transport client removal #3939

Conversation

prabhask5 commented Jan 11, 2024

Description

Issues Resolved

Testing

Check List

codecov bot commented Jan 12, 2024 • edited Loading

Codecov Report

prabhask5 commented Jan 12, 2024

peternied left a comment

Choose a reason for hiding this comment

stephen-crawford left a comment

Choose a reason for hiding this comment

DarshitChanpura left a comment

Choose a reason for hiding this comment

prabhask5 commented Jan 18, 2024 • edited Loading

prabhask5 commented Jan 18, 2024

stephen-crawford left a comment

Choose a reason for hiding this comment

DarshitChanpura commented Jan 18, 2024 • edited Loading

[Enhancement-3191] `transport_enabled` setting on an auth domain and authorizer may be unnecessary after transport client removal #3939

[Enhancement-3191] `transport_enabled` setting on an auth domain and authorizer may be unnecessary after transport client removal #3939

codecov bot commented Jan 12, 2024 •

edited

Loading

prabhask5 commented Jan 18, 2024 •

edited

Loading

DarshitChanpura commented Jan 18, 2024 •

edited

Loading